Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import dlc tracks #9

Merged
merged 53 commits into from
May 17, 2023
Merged

Import dlc tracks #9

merged 53 commits into from
May 17, 2023

Conversation

niksirbi
Copy link
Member

@niksirbi niksirbi commented Mar 9, 2023

In this PR I implement a function for loading deeplabcut pose estimation data from an .h5 or a .csv file.
The data is currently imported in a pd.DataFrame object, but in future PR the tracks will be saved in a custom Trajectory class (see #12 ).

This PR contains the first implemented feature, so alongside it, I have added:

  • logging (save circular logs in file)
  • unit tests
  • type validation via pydantic

@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@29e7b81). Click here to learn what that means.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main       #9   +/-   ##
=======================================
  Coverage        ?   96.29%           
=======================================
  Files           ?        4           
  Lines           ?       81           
  Branches        ?        0           
=======================================
  Hits            ?       78           
  Misses          ?        3           
  Partials        ?        0           

@niksirbi
Copy link
Member Author

I'm not sure how much I like the logging as a user. While saving logs to file would help debugging a lot, I'm not used to Python packages that log to console when using it interactively. I'd consider turning off the logging to console by default.

I like to see the console as well (as a developer), but I think you are right about the users. I turned it off by default but added a log_to_console boolean argument to configure_logging() function. Setting this to True also prints the log to the console using rich.logging.RichHandler()

@niksirbi
Copy link
Member Author

The result when loading from csv vs h5 differs. The csv result has an extra columns, and ends up with the body parts in the dataframe itself, but the .h5 has these in the column headings. I'd expect that the representation of the data should be the same regardless of which file format it was loaded from.

Yes, I am aware of that and was planning to take care of it later, since I plan to store the data into a custom object anyway. However, I might as well write a convenience function now, converting from the csv-imported to the h5-imported version.

@niksirbi
Copy link
Member Author

I have now also added a conversion from the CSV-imported table to the h5-imported one, so both types of files now return the same dataframe.

Copy link
Member

@adamltyson adamltyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around with the logging behaviour and all looks good to me:

  • If I use movement, then the logs end up in ~/.movement
  • if I use movement within another package with logging, then movement logs end up in the normal movement log, and the package log, but all other calls to logging only go to the package log.

This is what I'd expect, and I think what we should do with all of these packages.

I made some small suggestions.

README.md Outdated Show resolved Hide resolved
movement/log_config.py Outdated Show resolved Hide resolved
tests/test_unit/test_io.py Show resolved Hide resolved
movement/log_config.py Outdated Show resolved Hide resolved
tests/test_unit/test_io.py Outdated Show resolved Hide resolved
movement/log_config.py Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented May 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@niksirbi
Copy link
Member Author

I like to see the console as well (as a developer), but I think you are right about the users. I turned it off by default but added a log_to_console boolean argument to configure_logging() function. Setting this to True also prints the log to the console using rich.logging.RichHandler()

In the end, I purged the log_to_console boolean to simplify from the logging config and to remove the rich dependency (which I am not using for anything else at the moment)

@niksirbi niksirbi merged commit 11ae2d1 into main May 17, 2023
@niksirbi niksirbi deleted the import-DLC-tracks branch June 27, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core feature a core functionality that must be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants